-
Notifications
You must be signed in to change notification settings - Fork 916
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add updated_at column to objects' tables #1218
Add updated_at column to objects' tables #1218
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes! I believe this field uses the ISO 8601 string format YYYY-MM-DDTHH:mm:ss.sssZ
, so we should keep that consistent. And I know we can't change the field name, but when we render it I think it would look better to use a noun instead of including the at
proposition (i.e. Updated Time
instead of Updated at
).
src/plugins/dashboard/public/application/listing/dashboard_listing.js
Outdated
Show resolved
Hide resolved
...lugins/saved_objects_management/public/management_section/objects_table/components/table.tsx
Outdated
Show resolved
Hide resolved
...lugins/saved_objects_management/public/management_section/objects_table/components/table.tsx
Outdated
Show resolved
Hide resolved
There is an advanced setting in stack management that determines the date format of how dates should be pretty printed. We should be using that to format the date/time display. |
thanks, changed as requested |
Hey, all changes have been made inside this pr. |
7e81f6c
to
476d1da
Compare
src/plugins/visualize/public/application/utils/get_table_columns.tsx
Outdated
Show resolved
Hide resolved
src/plugins/dashboard/public/application/listing/dashboard_listing.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking to ensure stability for 2.0.0-rc1
Hi @RoyiSitbon, do you need any additional questions answered for this PR? |
Hey @tmarkley, thanks for checking. I replied above. |
476d1da
to
61116c6
Compare
88e68c4
to
05da790
Compare
Codecov Report
@@ Coverage Diff @@
## main #1218 +/- ##
==========================================
+ Coverage 66.55% 66.57% +0.02%
==========================================
Files 3170 3170
Lines 60321 60326 +5
Branches 9181 9182 +1
==========================================
+ Hits 40144 40162 +18
+ Misses 17984 17969 -15
- Partials 2193 2195 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for these improvements @RoyiSitbon! The code is clean and maintainable, that's much appreciated.
I think the last thing that would put the cherry on top is adding some additional unit tests as well as functional tests for this feature.
src/plugins/visualize/public/application/utils/get_table_columns.tsx
Outdated
Show resolved
Hide resolved
src/plugins/saved_objects/public/saved_object/saved_object_loader.ts
Outdated
Show resolved
Hide resolved
src/plugins/saved_objects/public/saved_object/saved_object_loader.test.ts
Show resolved
Hide resolved
src/plugins/visualize/public/application/utils/get_table_columns.tsx
Outdated
Show resolved
Hide resolved
5d60051
to
46f7e13
Compare
46f7e13
to
d5ba653
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you for the improvements!
Great then I think we can target to get this in to 2.3! Thank you so much @RoyiSitbon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've validated this locally and the change looks good! Glad that you exposed the updated_at
a global level so that every saved object type can utilize it. Sorry that this got sidetracked.
🙏 |
Signed-off-by: sitbubu <[email protected]>
Signed-off-by: sitbubu <[email protected]>
Signed-off-by: sitbubu <[email protected]>
Signed-off-by: sitbubu <[email protected]>
Signed-off-by: sitbubu <[email protected]>
Signed-off-by: sitbubu <[email protected]>
Signed-off-by: sitbubu <[email protected]>
Signed-off-by: sitbubu <[email protected]>
Signed-off-by: sitbubu <[email protected]>
7580cec
to
87e49ed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, I'm excited for this improvement!
// eslint-disable-next-line @typescript-eslint/naming-convention | ||
updated_at, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does it need to be updated_at
instead of updatedAt
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats the OpenSearch response. Don't think we can change that.
* Add updated_at columnb to objects' tables Signed-off-by: sitbubu <[email protected]> * Grammer and iso usage Signed-off-by: sitbubu <[email protected]> * Set updated at field from advanced settings Signed-off-by: sitbubu <[email protected]> * Use dateFormat instead of dateFormat:scaled Signed-off-by: sitbubu <[email protected]> * snapshot Signed-off-by: sitbubu <[email protected]> * Add updated_at to additional comment Signed-off-by: sitbubu <[email protected]> * Add unit-tests for updated_at as null undefined or unknown Signed-off-by: sitbubu <[email protected]> * Simplify test file Signed-off-by: sitbubu <[email protected]> * Simplified header and import from src Signed-off-by: sitbubu <[email protected]> Signed-off-by: sitbubu <[email protected]> (cherry picked from commit 8874afd)
* Add updated_at columnb to objects' tables Signed-off-by: sitbubu <[email protected]> * Grammer and iso usage Signed-off-by: sitbubu <[email protected]> * Set updated at field from advanced settings Signed-off-by: sitbubu <[email protected]> * Use dateFormat instead of dateFormat:scaled Signed-off-by: sitbubu <[email protected]> * snapshot Signed-off-by: sitbubu <[email protected]> * Add updated_at to additional comment Signed-off-by: sitbubu <[email protected]> * Add unit-tests for updated_at as null undefined or unknown Signed-off-by: sitbubu <[email protected]> * Simplify test file Signed-off-by: sitbubu <[email protected]> * Simplified header and import from src Signed-off-by: sitbubu <[email protected]> Signed-off-by: sitbubu <[email protected]> (cherry picked from commit 8874afd) Co-authored-by: Royi Sitbon <[email protected]>
Nice! |
* Add updated_at columnb to objects' tables Signed-off-by: sitbubu <[email protected]> * Grammer and iso usage Signed-off-by: sitbubu <[email protected]> * Set updated at field from advanced settings Signed-off-by: sitbubu <[email protected]> * Use dateFormat instead of dateFormat:scaled Signed-off-by: sitbubu <[email protected]> * snapshot Signed-off-by: sitbubu <[email protected]> * Add updated_at to additional comment Signed-off-by: sitbubu <[email protected]> * Add unit-tests for updated_at as null undefined or unknown Signed-off-by: sitbubu <[email protected]> * Simplify test file Signed-off-by: sitbubu <[email protected]> * Simplified header and import from src Signed-off-by: sitbubu <[email protected]> Signed-off-by: sitbubu <[email protected]> Signed-off-by: Sergey V. Osipov <[email protected]>
Signed-off-by: sitbubu [email protected]
Is your feature request related to a problem? Please describe.
We want to see more informative data inside the saved objects' tables,
one important column that answers our needs will be the “updated at” option.
After adding this option we will gain an additional advantage, i.e sort our objects by date.
Describe the solution you'd like to
We wish to add an additional “updated at” column to our management, visualizations, and dashboards tables. e.g. for the management table, we will define:
and then we will be able to see an additional column, like so:
The same solution will be implemented under the visualizations and dashboards tables, besides the fact that we will need to set our updated_at property inside the attributes prop since it uses us as the rendered object info for these tables.
Linked issue:
#1217